fix: align nudge summary token counts with tokenizer - #91
Open
Beatrice0377 wants to merge 4 commits into
Open
Conversation
ranxianglei
approved these changes
Aug 21, 2026
ranxianglei
left a comment
Owner
There was a problem hiding this comment.
Review: ✅ Approve — 2 independent agent reviews, both approve. Nits below are optional, none blocking.
Verified locally on the PR branch (branch current with master incl. v0.0.31):
npm run typecheck✅npm test→ 396/396 ✅ (master 391 + 5 new; PR body's "390/390" just reflects older master at authoring time — count is consistent)npm run build✅- CI green (pr-validation, test 22/24); mergeable CLEAN; both
issue #45named regression tests pass in isolation - Caller audit:
renderNudgeText/formatTierTargetBlockshave a single render path in src (exported via src/index.ts:35); no other estimator call sites remain
Design — implements #45's preferred option exactly:
- Precompute once in the decision layer (
pendingByTier, src/compress.ts) with the active injectedcountTokens; renderer stays dependency-free and consumes plain data (src/nudge-text.ts:39-46). Displayed number can no longer diverge from the arbitration number. pendingT2/pendingT3now reduce over the same per-block stats (src/compress.ts:961-967) — gate inputs are value-identical to master; no behavior change to arbitration.TierTargetBlockStatnot persisted onCompressionBlock— correct, sincecountTokensis host-injected and a stored value would go stale (rationale documented in src/types.ts).- Renderer joins stats by
blockIdvia Map, never by index — ordering drift cannot misattribute numbers. blockIds are unique by construction (monotonic counter, src/state.ts:16-23), so no duplicate-key risk.??correctly preserves a legitimate0token count. - Legacy
length / 4retained only as fallback for hand-built decisions without stats — backward compatible, and covered by a dedicated compat test.
Tests — good coverage: custom tokenizer that differs from both length/4 and defaultCountTokens (nice fake-fix guard), CJK-aware default tokenizer end-to-end (decision stats → pendingT2 aggregation → rendered text), legacy fallback, and ASCII-identity when stats equal the legacy estimate.
Nits (optional)
- No tier-3 stat assertion (code is symmetric with tier-2 — low risk).
- No test for a partial-stats array (one block missing from the Map → falls back per-block).
- Test-only
!non-null assertions — pre-existing pattern in this suite, fine as-is.
Fixes #45 as claimed. Thanks!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Tier-distillation nudges display each target block's summary token count, but the renderer estimated that value with
summary.length / 4.The decision layer already uses the injected
countTokensfunction for the same summaries. This makes the displayed compression payoff diverge from the value used for tier arbitration, especially for CJK text and custom tokenizers.Root Cause
pendingByTierhad access to the activecountTokens, butNudgeDecisiononly carried the targetCompressionBlocks.The per-block token counts were therefore lost at the decision boundary, forcing
renderNudgeTextto estimate them again withlength / 4.Fix
countTokens.pendingT2/pendingT3from those same precomputed values so arbitration and rendering share one token-counting source.NudgeDecisionastierTargetBlockStats.length / 4behavior as a compatibility fallback for manually constructed legacy decisions that do not provide stats.Scope / Non-goals
CompressionBlockis unchanged; derived token counts are not persisted into runtime state.renderNudgeText(decision, prompts?)keeps its existing signature.Tests
Added regression coverage for:
length / 4anddefaultCountTokens.Validation:
npm run typecheck: passed.npm run build: passed.git diff --check: passed.Compatibility
The new
NudgeDecision.tierTargetBlockStatsfield is optional.Existing manually constructed decisions without the field continue to use the previous renderer estimate, while normal kernel-produced tier-2/tier-3 decisions provide tokenizer-consistent stats.
Limitations
Legacy/manual decisions that omit
tierTargetBlockStatsintentionally retain the previous fallback estimate for backward compatibility.Fixes #45